Skip to content

Conversation

@AlexandruGG
Copy link

@AlexandruGG AlexandruGG commented May 27, 2021

Description

This PR adds type descriptors for Carbon date types Carbon and CarbonImmutable using the ReflectionDescriptor.

Made possible after: briannesbitt/Carbon#2344.

Features / Changes

  • Registers descriptors in extension.neon
  • Adds test cases in MyBrokenEntity.php

@AlexandruGG
Copy link
Author

@ondrejmirtes hello!

Firstly, thank you for this extension and PHPStan in general, it's very useful! 👍

I'm looking to add type descriptors for Carbon date types and wanted to check two things first:

  1. Is this something that you consider within the scope of this extension? Or is it more something like a custom type that the end-user should add themselves. I thought it would be useful because the library is widely used

  2. I'm having trouble running a fresh composer install in this project. I tried various things but still could not get it working. Am I missing something?

$ composer install
No lock file found. Updating dependencies instead of installing from lock file. Use composer update over composer install if you do not have a lock file.
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - phpstan/phpstan-doctrine is present at version 0.12.x-dev and cannot be modified by Composer
    - Conclusion: don't install doctrine/common 3.1.x-dev (conflict analysis result)
    - Conclusion: don't install doctrine/common 3.2.x-dev (conflict analysis result)
    - Conclusion: don't install doctrine/common 3.0.3 (conflict analysis result)
    - Conclusion: don't install doctrine/common 3.1.0 (conflict analysis result)
    - Conclusion: don't install doctrine/common 3.1.1 (conflict analysis result)
    - Conclusion: don't install doctrine/common 3.1.2 (conflict analysis result)
    - Root composer.json requires doctrine/persistence ^1.1 || ^2.0 -> satisfiable by doctrine/persistence[v1.1.0, ..., 1.4.x-dev, 2.0.0, ..., 2.3.x-dev].
    - Root composer.json requires doctrine/mongodb-odm ^1.3 || ^2.1 -> satisfiable by doctrine/mongodb-odm[1.3.0-RC1, ..., 1.3.x-dev, 2.1.0, ..., 2.3.x-dev].
    - doctrine/orm[2.9.1, ..., 2.10.x-dev] require doctrine/common ^3.0.3 -> satisfiable by doctrine/common[3.0.3, ..., 3.2.x-dev].
    - Conclusion: don't install doctrine/common 3.0.x-dev (conflict analysis result)
    - Root composer.json requires doctrine/orm ^2.9.1 -> satisfiable by doctrine/orm[2.9.1, 2.9.x-dev, 2.10.x-dev].```

@ondrejmirtes
Copy link
Member

Hi, can you link to those Carbon Doctrine types you want to describe? Thanks.

As for your installation problem:

  • Make sure you use Composer 2
  • Try deleting composer.lock and vendor/.
  • Try it with --ignore-platform-reqs

Check out the CI scripts, it's not doing anything special...

@AlexandruGG
Copy link
Author

Hi, can you link to those Carbon Doctrine types you want to describe? Thanks.

As for your installation problem:

  • Make sure you use Composer 2
  • Try deleting composer.lock and vendor/.
  • Try it with --ignore-platform-reqs

Check out the CI scripts, it's not doing anything special...

Hey, thanks for the fast reply.

I managed to run the installation with --ignore-platform-reqs 👍 .

Regarding the types, here they are:

They both use the CarbonTypeConverter to do the conversions from/to database.

@ondrejmirtes
Copy link
Member

The easiest way forward would be for the library to add proper typehints - the methods convertToPHPValue and convertToDatabaseValue don't have the correct return typehints. And after that registering the ReflectionDescriptor would be sufficient. (https://github.com/phpstan/phpstan-doctrine#reflectiondescriptor).

Alternatively an implementation like https://github.com/phpstan/phpstan-doctrine/blob/master/src/Type/Doctrine/Descriptors/Ramsey/UuidTypeDescriptor.php would also work.

@AlexandruGG
Copy link
Author

AlexandruGG commented May 28, 2021

Thanks for the details.

Looks like not even the Doctrine date types have those type hints 🤷‍♂️ .

Also it might be difficult due to the way the code is structured as the converter is used for both Carbon and CarbonImmutable. The return type of convertToPHPValue will be the class instance returned by getCarbonClassName, which gets overridden in here. Is it possible to convey this properly via type hints?

The above is mostly why I thought a custom descriptor like DateTimeImmutableType would be the easiest, unless I'm missing something.

@ondrejmirtes
Copy link
Member

Oh, in that case we need a custom DoctrineTypeDescriptor implementation here :) So please submit the PR, thanks :)

@AlexandruGG AlexandruGG changed the title [WIP] Carbon Type Descriptors Carbon Type Descriptors May 28, 2021
@AlexandruGG AlexandruGG marked this pull request as ready for review May 28, 2021 13:39
@AlexandruGG
Copy link
Author

Oh, in that case we need a custom DoctrineTypeDescriptor implementation here :) So please submit the PR, thanks :)

Added now, please run the workflow as it's my first time contributing here :)

@AlexandruGG AlexandruGG force-pushed the feature/carbon-type-descriptors branch from 3f98b48 to 1e3c1ea Compare June 9, 2021 17:54
@AlexandruGG AlexandruGG changed the title Carbon Type Descriptors Carbon Type Descriptors via ReflectionDescriptor Jun 9, 2021
@AlexandruGG AlexandruGG marked this pull request as ready for review June 9, 2021 17:57
@AlexandruGG
Copy link
Author

@ondrejmirtes hey 👋 , this is updated and ready for re-review now

@AlexandruGG AlexandruGG requested a review from ondrejmirtes June 9, 2021 17:57
@ondrejmirtes ondrejmirtes merged commit c35261f into phpstan:master Jun 10, 2021
@AlexandruGG
Copy link
Author

thank you! 💯

@AlexandruGG AlexandruGG deleted the feature/carbon-type-descriptors branch June 10, 2021 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants